Add Codex device-code sign-in#1045
Conversation
Adds a device-code Codex login path alongside the existing browser OAuth flow while reusing the current Codex credential storage, profile, and runtime routing.
There was a problem hiding this comment.
Pull request overview
Adds a new Codex “device code” sign-in path alongside the existing Codex OAuth flow, reusing the same secure credential storage and Codex runtime activation behavior.
Changes:
- Introduce a Codex device-code API flow (device-code request + token polling) and a React hook to drive the UI flow and persistence.
- Add “Codex Device Code” as a selectable option in both ProviderManager and
/providerwizard flows. - Update provider validation hints and preset ordering expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/providerValidation.ts | Updates Codex auth validation hints to mention device-code sign-in. |
| src/services/api/codexDeviceFlow.ts | Adds device-code request/polling implementation for Codex auth. |
| src/components/useCodexDeviceCodeFlow.ts | Adds React hook orchestrating device-code sign-in, browser open, polling, and secure persistence. |
| src/components/ProviderManager.tsx | Adds a new “Codex Device Code” setup screen and preset option. |
| src/components/ProviderManager.test.tsx | Updates preset ordering expectations to include “Codex Device Code”. |
| src/commands/provider/provider.tsx | Adds a new /provider wizard step for Codex device-code sign-in. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (canUseCodexOAuth) { | ||
| options.splice(7, 0, { | ||
| value: 'codex-device-code', | ||
| label: 'Codex Device Code', | ||
| description: | ||
| 'Sign in with a device code and store Codex credentials securely', | ||
| }) | ||
| options.splice(6, 0, { | ||
| value: 'codex-oauth', |
| export function useCodexDeviceCodeFlow(options: { | ||
| onAuthenticated: ( | ||
| tokens: CodexOAuthTokens, | ||
| persistCredentials: PersistCodexCredentials, | ||
| ) => void | Promise<void> | ||
| deps?: CodexDeviceCodeFlowDependencies | ||
| }): CodexDeviceCodeFlowStatus { |
| const response = await fetchFn(CODEX_DEVICE_CODE_URL, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| body, | ||
| }) |
| const response = await options.fetchImpl(CODEX_REFRESH_URL, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| body, | ||
| signal: options.signal, |
|
please address copilot's comments |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Additional Review Response for PR #1045
1. Missing interval is treated as a hard failure
File: src/services/api/codexDeviceFlow.ts
The current device-code response parser rejects otherwise valid successful responses when the provider omits interval. That behavior is inconsistent with the rest of the implementation, which already falls back to a default polling interval of five seconds when interval is absent or non-positive. In practice, this means legitimate device-code sign-in attempts can fail immediately even though the flow already has a safe default.
I recommend accepting responses that omit interval and normalizing that case to the existing five-second fallback.
2. Device-code reauthentication creates redundant Codex profiles
File: src/components/ProviderManager.tsx
The new device-code branch always creates a fresh provider profile instead of reusing the stored Codex-linked profile when one already exists. This differs from the existing Codex OAuth implementation, which updates the prior profile in place. The surrounding credential model still appears to treat Codex secure storage as a single linked login with a single associated profile id, so repeated device-code sign-ins create redundant Codex profiles even though the underlying secure store only tracks one linked profile. The current logout and delete cleanup paths then only follow the most recently stored profile id.
I recommend mirroring the existing OAuth behavior by resolving the stored Codex-linked profile first and updating it when present instead of always creating a new profile.
3. slow_down without an interval does not increase the polling delay
File: src/services/api/codexDeviceFlow.ts
When the token endpoint returns slow_down without an accompanying interval, the polling loop continues at its previous cadence rather than backing off. This can leave the client in a repeated rate-limited state and makes the device-code flow less resilient under real-world server behavior.
I recommend applying a default backoff increase for slow_down even when the response does not provide an explicit interval, such as increasing the current interval by five seconds.
Conclusion
These issues appear reasonably contained, but I recommend addressing them before merge. The first can cause the new sign-in path to fail outright, while the latter two introduce avoidable long-term behavior issues in repeated or rate-limited authentication flows.
|
Thanks for the review. I updated the PR to address the feedback: Updates
Validation
|
| !Number.isFinite(expiresIn) || | ||
| !Number.isFinite(interval) | ||
| ) { | ||
| throw new CodexDeviceFlowError( | ||
| 'Codex device-code response was missing required fields.', | ||
| ) | ||
| } | ||
|
|
||
| return { | ||
| deviceCode, | ||
| userCode, | ||
| verificationUri, | ||
| verificationUriComplete, | ||
| expiresIn, | ||
| interval: interval > 0 ? interval : 5, |
| while (Date.now() - startedAt < timeoutMs) { | ||
| const result = await pollTokenOnce({ | ||
| deviceCode, | ||
| clientId, | ||
| fetchImpl: fetchFn, | ||
| signal: options?.signal, | ||
| requestTimeoutMs: options?.requestTimeoutMs, | ||
| }) | ||
|
|
|
|
||
| void (async () => { | ||
| try { | ||
| const deviceCode = await requestDeviceCode() |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Follow-up review for PR #1045
Thanks for the updates. I pulled and reviewed the latest PR head (5c686fb6249973b82dc92b8acd79f2cc2bf14dc6). Some earlier feedback was addressed, but the PR is not complete yet.
Intent check
The PR summary says this should add a Codex Device Code provider sign-in option, reuse existing Codex secure credential storage/profile activation/runtime routing, and keep provider UI ordering/validation aligned. The findings below are scoped to that intent: they either break standards-aligned device-code polling, diverge from the existing Codex OAuth profile reuse model, or leave the new ProviderManager path unverified.
Requested changes still outstanding
-
Missing
intervalis still treated as a hard failureFile:
src/services/api/codexDeviceFlow.tsparseDeviceCodeResponse()still requiresNumber.isFinite(interval)before accepting the device-code response. If the provider omitsinterval, the request still throwsCodex device-code response was missing required fields.even though the return path already has a fallback of5. This leaves the prior requested change unresolved.This is also against the OAuth 2.0 Device Authorization Grant behavior:
intervalis optional, and clients use 5 seconds by default when it is absent.Please accept an omitted or invalid
intervaland normalize it to the existing default polling interval. -
Device-code reauthentication still creates redundant Codex profiles
File:
src/components/ProviderManager.tsxThe device-code setup branch still calls
addProviderProfile(...)unconditionally. The OAuth branch immediately below it checks the stored Codex-linked profile id and updates that existing profile when present. Device-code sign-in should mirror that behavior so repeated Codex sign-ins do not create duplicate profiles while secure storage only tracks one linked profile id.Please reuse/update the existing stored Codex profile when present, then persist credentials with that profile id.
-
slow_downwithout anintervalstill does not increase polling delayFile:
src/services/api/codexDeviceFlow.tspollCodexDeviceToken()only applies theinterval + 5backoff whenresult.state === 'slow_down' && result.interval. If the token endpoint returnsslow_downwithout aninterval, the code falls through and keeps the previous polling cadence.Device-code polling expects
slow_downto increase the interval by five seconds for this and subsequent requests, even without a server-supplied replacement interval.Please increase the polling delay on every
slow_down, using the returned interval when present and otherwise applying the default five-second backoff.
Copilot comments status
- Resolved: Codex OAuth and Codex Device Code are now adjacent in the preset list.
- Resolved: Hook-level tests were added for
useCodexDeviceCodeFlow. - Partially addressed: Request-level timeout plumbing was added for the device-code request and polling fetches, but the new timeout tests in
src/services/api/codexDeviceFlow.test.tshang locally when run directly. - Still valid: Copilot's latest comment about optional
intervalis valid. - Still valid: Copilot's latest comment about passing the hook abort signal into the initial
requestCodexDeviceCode()call is valid. The hook creates anAbortController, but callsrequestDeviceCode()with no{ signal: controller.signal }, so unmounting during the first request does not cancel it. - Still valid / needs decision: Copilot's latest comment about respecting the initial polling interval before the first token request appears valid for device-code semantics. The current loop polls immediately, then sleeps only after a pending response.
Additional review findings
-
Device-code timeout tests hang
File:
src/services/api/codexDeviceFlow.test.tsThe two tests that expect a hanging request to abort via
requestTimeoutMs: 1did not complete locally:requestCodexDeviceCode > aborts a hanging request after the per-request timeoutpollCodexDeviceToken > aborts a hanging polling request after the per-request timeout
The other targeted tests in that file pass when filtered individually, and
src/components/useCodexDeviceCodeFlow.test.tsxpasses. Please adjust the timeout implementation or tests so the new timeout coverage is reliable under Bun.This one is primarily a validation/reliability issue. It does not prove the runtime timeout path is broken by itself, but it does mean the PR's newly added timeout coverage is not currently usable as evidence.
-
ProviderManager has no device-code integration coverage
File:
src/components/ProviderManager.test.tsxThe PR adds the ProviderManager device-code screen and profile persistence path, but the tests only update preset ordering. There is existing coverage for the OAuth first-run path, but no matching test that exercises Codex Device Code through ProviderManager. This missing coverage is exactly where the duplicate-profile regression lives.
Please add a ProviderManager test for device-code sign-in that covers profile creation, active profile selection, stored Codex profile linkage, and repeat sign-in reusing the existing Codex profile.
Validation run
bun install --frozen-lockfile: passed.bun test src/components/useCodexDeviceCodeFlow.test.tsx: passed, 5 tests.bun test src/services/api/codexDeviceFlow.test.ts: timed out locally.- Filtered passing checks from
src/services/api/codexDeviceFlow.test.ts: successful parse, successful poll, and caller abort signal tests passed.
|
@kevincodex1 I'm not sure we should keep down this route for now. |
|
I vote we close this PR. It's incomplete and a beta feature which can be removed without notice. |
|
Hi bro @Pedromdsn thank you so much for your time working on this, but since its still a beta feature lets halt for now. will be closing this PR and once its stable already you can reopen and continue working on it. |
Summary
Test plan
bun run buildbun test src/commands/provider/provider.test.tsx src/components/ProviderManager.test.tsx src/utils/providerValidation.tsbun test src/services/api/providerConfig.codexSecureStorage.test.ts src/services/api/providerConfig.runtimeCodexCredentials.test.ts src/utils/codexCredentials.test.tsNotes
bun testhas unrelated environment-sensitive failures inStartupScreen.test.tsandsdk-context-isolation.test.tswhen Codex/OpenAI env vars are set locally; those tests pass when the Codex env is unset.bun run typecheckcurrently fails with unrelated baseline errors outside these changed files.